Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove an arbitrary sleep of 500 milliseconds. #1106

Closed
wants to merge 2 commits into from

Conversation

clalancette
Copy link
Contributor

The code below already attempts to take the messages for up to 10 seconds, so there is no reason for this wait.

The code below already attempts to take the messages
for up to 10 seconds, so there is no reason for this
wait.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Linux repeated Build Status

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. (it is weird that we have this sleep only in here... it can deal with do...while wait loop anyway.)

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

So it turns out that we actually do indeed need this 500 millisecond timeout. The point of the test is to test that we are only taking 3 of the messages when 5 are available. And for that, we need to make sure the 5 are actually available, which this 500 millisecond sleep (mostly) accomplishes.

Honestly, though, what we really need is another API to tell us when we have 5 subscription messages available to take. That way we can just wait until that happens, and then get rid of the sleep. Since that would involve work at the RMW layer, I decided to just add a TODO to this effect. I don't love it, but at least we won't wonder why this is in here again.

@sloretz
Copy link
Contributor

sloretz commented Oct 12, 2023

[...] what we really need is another API to tell us when we have 5 subscription messages available to take [...]

Opened issue describing that feature: #1109

Closing for now.

@sloretz sloretz closed this Oct 12, 2023
@sloretz sloretz deleted the clalancette/remove-arbitrary-sleep branch October 12, 2023 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants